-
-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix NoMethodError for --pr option (caused by module_options = nil
) / introduce --noop
#188
Conversation
The newly added tests prove the bug reported with issue voxpupuli#187. This change also adds --noop support for the --pr option.
module_options = nil
)module_options = nil
)
bb26f94
to
d75e971
Compare
This should be ready for merging now. Can you take a look again? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the following pattern would be easier:
do_prep_work
if options[:noop]
$stdout.puts "Using no-op. Would do real work"
return
end
do_real_work
I don't know the answer - there are really good arguments for both.
4541081
to
c948011
Compare
I have applied your suggested changes to the GitLab pr module. That makes the code structure flatter, which is better. Nice, thank you! In the GitHub pr module the code is different: The code, which sets the labels, needs to run afterwards and has a separate |
Any more corrections needed? Otherwise, I'd be happy if we could merge the changes. 🤞 |
I was going to say the same actually. I prefer short circuits for this kind of logic. |
c948011
to
3e7685e
Compare
Align implementation for GitHub PRs and GitLab MRs
3e7685e
to
ebc888f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raphink I think this looks good, but since reviewed it too I'll let you do the final review.
Ok for me then |
module_options = nil
)module_options = nil
) / introduce --noop
Adds
--noop
support for the--pr
option.Adds feature tests for both GitHub PRs and GitLab MRs to prove the bug reported with issue #187.
Fixes #187